Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

doc: value choice for imitating the old behavior of http.Server.keepAliveTimeout #17660

Closed
wants to merge 2 commits into from
Closed

doc: value choice for imitating the old behavior of http.Server.keepAliveTimeout #17660

wants to merge 2 commits into from

Conversation

TysonAndre
Copy link

Checklist

Documenting the best way to imitate the old behavior saves time for people
migrating from older versions.
(E.g. for unexpected ECONNRESET)

It isn't immediately obvious if earlier nodejs versions behaved the same
way as nodejs 8 does with keepAliveTimeout = 0.
From 0aa7ef5, it seems like they behave
the same way.

Related to issues such as #13391 that show up when migrating to node 8

Affected core subsystem(s)

doc

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. labels Dec 13, 2017
Trott
Trott reviewed Dec 14, 2017
View reviewed changes
doc/api/http.md Outdated
@@ -919,6 +919,8 @@ timeout has fired, it will reset the regular inactivity timeout, i.e.,
[`server.timeout`][].

A value of 0 will disable the keep-alive timeout behavior on incoming connections.
A value of 0 makes the http server behaves similarly to earlier nodejs versions,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change behaves to behave and nodejs to Node.js.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, while we're in here, it would be good to put backticks around 0 so it's `0`?

Trott
Trott reviewed Dec 14, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @TysonAndre! Welcome and thanks for the pull request. I have no opinion on whether the actual change should go through or not--it's not clear to me that catering to people looking to emulate old versions is good for the documentation. But if it does go through, there are some grammar/style corrections that will need to happen. I've left a pair of comments. Please take a look. Thanks again!

@TysonAndre-tmg @TysonAndre
doc: value choice for imitating the old behavior of http.Server.keepA…
ed9287e 
…liveTimeout

Documenting the best way to imitate the old behavior saves time for people
migrating from older versions.
(E.g. for unexpected ECONNRESET)

It isn't immediately obvious if earlier nodejs versions behaved the same
way as nodejs 8 does with keepAliveTimeout = 0.
From 0aa7ef5, it seems like they behave
the same way.

Related to issues such as #13391 that show up when migrating to node 8
Trott
Trott approved these changes Dec 14, 2017
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making the change additive like this is 👍 by me. Optional improvement: Might be good to include the exact version in which it changed. So instead of earlier Node.js versions it could be Node.js versions prior to 8.0.0 or whatever the right version number is.

addaleax
addaleax approved these changes Dec 27, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if @apapirovski is happy with it

apapirovski
apapirovski approved these changes Dec 27, 2017
View reviewed changes
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from a technical perspective. Maybe not having the "A value of 0" repeat would be possible but that can be adjusted in a different PR if someone feels passionate enough about it.

@jasnell
Copy link
Member

jasnell commented Dec 28, 2017

CI lite: https://ci.nodejs.org/job/node-test-pull-request-lite/52/

jasnell pushed a commit that referenced this pull request Dec 28, 2017
@TysonAndre-tmg @jasnell
doc: doc imitating the old behavior of http.Server.keepAliveTimeout
214bbb5 
Documenting the best way to imitate the old behavior saves time for people
migrating from older versions. (E.g. for unexpected ECONNRESET)

It isn't immediately obvious if earlier nodejs versions behaved the same
way as nodejs 8 does with keepAliveTimeout = 0.
From 0aa7ef5, it seems like they behave
the same way.

Related to issues such as #13391 that show up when migrating to node 8

PR-URL: #17660
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@jasnell
Copy link
Member

jasnell commented Dec 28, 2017

Landed in 214bbb5

@jasnell jasnell closed this Dec 28, 2017
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@TysonAndre-tmg @MylesBorins
doc: doc imitating the old behavior of http.Server.keepAliveTimeout
073c0f7 
Documenting the best way to imitate the old behavior saves time for people
migrating from older versions. (E.g. for unexpected ECONNRESET)

It isn't immediately obvious if earlier nodejs versions behaved the same
way as nodejs 8 does with keepAliveTimeout = 0.
From 0aa7ef5, it seems like they behave
the same way.

Related to issues such as #13391 that show up when migrating to node 8

PR-URL: #17660
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@TysonAndre-tmg @MylesBorins
doc: doc imitating the old behavior of http.Server.keepAliveTimeout
98eb989 
Documenting the best way to imitate the old behavior saves time for people
migrating from older versions. (E.g. for unexpected ECONNRESET)

It isn't immediately obvious if earlier nodejs versions behaved the same
way as nodejs 8 does with keepAliveTimeout = 0.
From 0aa7ef5, it seems like they behave
the same way.

Related to issues such as #13391 that show up when migrating to node 8

PR-URL: #17660
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@TysonAndre-tmg @MylesBorins
doc: doc imitating the old behavior of http.Server.keepAliveTimeout
4850c87 
Documenting the best way to imitate the old behavior saves time for people
migrating from older versions. (E.g. for unexpected ECONNRESET)

It isn't immediately obvious if earlier nodejs versions behaved the same
way as nodejs 8 does with keepAliveTimeout = 0.
From 0aa7ef5, it seems like they behave
the same way.

Related to issues such as #13391 that show up when migrating to node 8

PR-URL: #17660
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
gibfahn pushed a commit that referenced this pull request Jan 24, 2018
@TysonAndre-tmg @gibfahn
doc: doc imitating the old behavior of http.Server.keepAliveTimeout
de85204 
Documenting the best way to imitate the old behavior saves time for people
migrating from older versions. (E.g. for unexpected ECONNRESET)

It isn't immediately obvious if earlier nodejs versions behaved the same
way as nodejs 8 does with keepAliveTimeout = 0.
From 0aa7ef5, it seems like they behave
the same way.

Related to issues such as #13391 that show up when migrating to node 8

PR-URL: #17660
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@apapirovski apapirovski apapirovski approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@TysonAndre @jasnell @apapirovski @Trott @addaleax @lpinca @gibfahn @nodejs-github-bot @TysonAndre-tmg